Add sha256 uniqueness to CollectionVersion #1815

mdellweg · 2024-04-15T11:40:15Z

No description provided.

pulp_ansible/tests/functional/api/role/test_crud_distribution.py

mdellweg · 2024-04-16T12:51:01Z

pulp_ansible/app/tasks/collections.py

-        try:
-            await sync_to_async(artifact.save)()
-        except IntegrityError:
-            artifact = Artifact.objects.get(sha256=artifact.sha256)


This line would never have worked in an async context.

gerrod3

I know this is based on my original work on this, but there was discussion back then on how the sha256 for the Collection should be the digest of the MANIFEST.json file inside the tarball. I actually never got around to finishing all of the changes to my PR, so most of my comments here is me pointing out all the areas I didn't change it.

#1119 (comment)

gerrod3 · 2024-04-19T13:49:14Z

pulp_ansible/app/serializers.py

-                )
-
-        return col
+        return CollectionVersion.objects.filter(sha256=validated_data["sha256"]).first()


Nice! This is so much more simple.

gerrod3 · 2024-04-19T13:50:21Z

pulp_ansible/app/serializers.py

+        # There was an autogenerated validator rendering sha256 required.
+        validators = []


This is actually a really annoying feature of DRF. It tripped me up a lot when I was originally adding domains.

gerrod3 · 2024-04-19T13:51:40Z

pulp_ansible/app/tasks/collections.py

-        except IntegrityError:
-            artifact = Artifact.objects.get(sha256=artifact.sha256)
+        if existing_artifact := await Artifact.objects.filter(sha256=artifact.sha256).afirst():
+            existing_artifact.touch()


Does this need to be wrapped in a sync_to_async or do we have a atouch?

pulp_ansible/app/migrations/0057_collectionversion_sha256_migrate.py

pulp_ansible/app/serializers.py

pulp_ansible/app/tasks/collections.py

gerrod3 · 2024-04-19T14:00:26Z

pulp_ansible/app/tasks/collections.py

-        for key in ["description", "documentation", "homepage", "issues", "repository"]:
-            if collection_info[key] is None:
-                collection_info.pop(key)
+    collection, created = Collection.objects.get_or_create(


Isn't this a database operation? This breaks the comment about metadata_only=True.

pulp_ansible/app/tasks/collections.py

gerrod3 · 2024-04-19T14:04:20Z

pulp_ansible/app/tasks/collections.py

+                if artifact_file_name := d_artifact_files.get(d_artifact):
+                    artifact_file = open(artifact_file_name, mode="rb")


I don't understand this logic. Can you help explain it? Edit Looking at the original PR I was the one who wrote this... Well present me can't remember what past me intended with this logic.

pulp_ansible/app/tasks/upload.py

mdellweg · 2024-04-22T06:42:28Z

I know this is based on my original work on this, but there was discussion back then on how the sha256 for the Collection should be the digest of the MANIFEST.json file inside the tarball. I actually never got around to finishing all of the changes to my PR, so most of my comments here is me pointing out all the areas I didn't change it.

#1119 (comment)

The reason raised for this idea was: A user can rebuild the collection and reupload it. After that they may be confused to find two content units with the same (namespace, name, version) but different artifacts. The whole idea of this pr is to allow having different artifacts claiming the same (namespace, name, version). I as a user would on the other hand be most suspicious if i built a collection, uploaded it downloaded it again and then got a different artifact.
I'd like to revisit that discussion, as it probably decides on whether we can ever implement on_demand sync in a sane way.

gerrod3

Small suggestions, everything else looks good.

gerrod3 · 2024-11-11T16:10:40Z

pulp_ansible/app/management/commands/datarepair-ansible-collection-sha256.py

+        if count == 0:
+            return
+
+        for unit in unit_qs.iterator():


Should we add theprefetch_related here to reduce the number of queries being made?

gerrod3 · 2024-11-11T16:20:47Z

pulp_ansible/app/tasks/collections.py

-            relative_path=collection_version.relative_path,
-        )
+        collection_version, tags = await sync_to_async(create_collection_from_importer)(metadata)
+        await _save_collection_version(collection_version, artifact, tags)
    except ValidationError as e:
        if "unique" in str(e):


You need to update the CollectionVersion.get below on line 148 to include the sha256, also can make it use aget.

Actually, I think this is the place where we still have the old behaviour, and we should not change that.
Remember this is not fixing the issue, but preparing the database to fix the issue with proper lookup later.

gerrod3 · 2024-11-11T16:32:49Z

pulp_ansible/tests/unit/utils.py

        collection_versions.append(cv)

-        if build_artifacts:


build_artifacts is no longer being used in this method, can we remove it from the signature?

This should be ZDU compatible. The transition will be completed with the next y-release. This commit only adds the field an starts to populate it. It comes with a data repair command to help transition to an actual uniqueness constraint in the next release. relates: pulp#1052 Co-authored-by: Matthias Dellweg <[email protected]>

gerrod3

Hurray! One step closer.

github-actions bot added the multi-commit label Apr 15, 2024

mdellweg force-pushed the 1052_uniqueness branch 8 times, most recently from e97a863 to 775fb7c Compare April 16, 2024 10:22

mdellweg commented Apr 16, 2024

View reviewed changes

pulp_ansible/tests/functional/api/role/test_crud_distribution.py Outdated Show resolved Hide resolved

mdellweg force-pushed the 1052_uniqueness branch 2 times, most recently from 03bef99 to baf54a1 Compare April 16, 2024 12:33

mdellweg commented Apr 16, 2024

View reviewed changes

mdellweg force-pushed the 1052_uniqueness branch 2 times, most recently from 25ac028 to d4e93f5 Compare April 19, 2024 13:34

github-actions bot removed the multi-commit label Apr 19, 2024

mdellweg mentioned this pull request Apr 19, 2024

[ZDU] Finalize Collection Version sha256 [PULP-38] #1818

Open

mdellweg force-pushed the 1052_uniqueness branch from d4e93f5 to d2949b2 Compare April 19, 2024 13:55

gerrod3 requested changes Apr 19, 2024

View reviewed changes

mdellweg force-pushed the 1052_uniqueness branch 4 times, most recently from 80667d2 to b09eed7 Compare May 7, 2024 10:48

mdellweg linked an issue May 8, 2024 that may be closed by this pull request

Content should have a digest in the (system-)uniqueness constraints #1052

Closed

mdellweg force-pushed the 1052_uniqueness branch 2 times, most recently from dcd8b05 to 2733da5 Compare May 13, 2024 14:31

mdellweg force-pushed the 1052_uniqueness branch 2 times, most recently from d857fe1 to b4ded81 Compare May 27, 2024 13:19

mdellweg force-pushed the 1052_uniqueness branch from b4ded81 to 1149713 Compare July 11, 2024 10:33

mdellweg force-pushed the 1052_uniqueness branch 2 times, most recently from 70cafff to 27c3a55 Compare September 13, 2024 12:48

github-actions bot added the multi-commit label Sep 13, 2024

mdellweg force-pushed the 1052_uniqueness branch from 27c3a55 to 7ef8820 Compare September 13, 2024 15:08

github-actions bot removed the multi-commit label Sep 13, 2024

mdellweg force-pushed the 1052_uniqueness branch from 7ef8820 to c44d205 Compare September 26, 2024 11:07

github-actions bot added the no-changelog label Sep 26, 2024

mdellweg force-pushed the 1052_uniqueness branch 4 times, most recently from 7aead6d to 834a3bf Compare September 27, 2024 14:39

mdellweg force-pushed the 1052_uniqueness branch 3 times, most recently from 10cfcb0 to 5fb7a0e Compare October 10, 2024 14:03

mdellweg force-pushed the 1052_uniqueness branch from 5fb7a0e to 845c7e6 Compare October 30, 2024 07:31

github-actions bot added multi-commit no-issue labels Oct 30, 2024

Remove line noise

5a4c32a

mdellweg force-pushed the 1052_uniqueness branch 4 times, most recently from 52973f6 to 58f25f4 Compare November 6, 2024 13:44

mdellweg marked this pull request as ready for review November 6, 2024 15:21

mdellweg enabled auto-merge (rebase) November 6, 2024 15:21

gerrod3 requested changes Nov 11, 2024

View reviewed changes

mdellweg force-pushed the 1052_uniqueness branch from 58f25f4 to 566ebbf Compare November 11, 2024 16:55

gerrod3 approved these changes Nov 11, 2024

View reviewed changes

mdellweg merged commit 544e468 into pulp:main Nov 11, 2024
12 checks passed

mdellweg deleted the 1052_uniqueness branch November 11, 2024 18:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sha256 uniqueness to CollectionVersion #1815

Add sha256 uniqueness to CollectionVersion #1815

mdellweg commented Apr 15, 2024

mdellweg Apr 16, 2024

gerrod3 left a comment

gerrod3 Apr 19, 2024

gerrod3 Apr 19, 2024

gerrod3 Apr 19, 2024

gerrod3 Apr 19, 2024 •

edited

Loading

gerrod3 Apr 19, 2024 •

edited

Loading

mdellweg commented Apr 22, 2024

gerrod3 left a comment

gerrod3 Nov 11, 2024

gerrod3 Nov 11, 2024

mdellweg Nov 11, 2024

gerrod3 Nov 11, 2024

gerrod3 left a comment

		# There was an autogenerated validator rendering sha256 required.
		validators = []

		if artifact_file_name := d_artifact_files.get(d_artifact):
		artifact_file = open(artifact_file_name, mode="rb")

Add sha256 uniqueness to CollectionVersion #1815

Add sha256 uniqueness to CollectionVersion #1815

Conversation

mdellweg commented Apr 15, 2024

Choose a reason for hiding this comment

gerrod3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerrod3 Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

gerrod3 Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

mdellweg commented Apr 22, 2024

gerrod3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerrod3 left a comment

Choose a reason for hiding this comment

gerrod3 Apr 19, 2024 •

edited

Loading

gerrod3 Apr 19, 2024 •

edited

Loading